feat: delegate issue triage to subject agents - #584

Review: feat: delegate issue triage to subject agents

I reviewed the current head (factory/triage-subject-turns) against main, ran the full test suite, and checked catalog/bundle consistency.

What the PR does

The github-issue-triage extension is refactored from an SDK-coupled automation that ran its own conversation runtime into a lightweight scanner that delegates triage work to Automation's subject-turn API. The worker no longer imports openhands.sdk (RemoteConversation, RemoteWorkspace, register_default_tools) or manages AGENT_SERVER_URL / SESSION_API_KEY / AUTOMATION_CONVERSATION_ID. Instead, submit_subject_turn() posts each changed issue to AUTOMATION_SUBJECT_TURN_URL with a Bearer $AUTOMATION_RUN_TOKEN credential, keyed by GitHub's immutable repository ID + issue number, and idempotent via a content digest.

Verification performed

• Full test suite: 823 passed, 24 skipped (includes the 3 new triage delivery tests, all catalog/sync/schema tests).
• Ruff: clean on worker.py and test_github_triage_delivery.py.
• Generated catalog: npm run build:automations and npm run build:skills produce no diff — bundle-index.js and skills/index.js are in sync with source files.
• Bundle-index ↔ source: confirmed worker.py and github_client.py in bundle-index.js match the source files byte-for-byte.

Code quality observations (positive)

• Clean separation of concerns: the scanner is a plain host command; only the delegated agent receives a conversation runtime. SDK imports, workspace lifecycle, and session management are fully removed.
• Idempotency: subject_key uses {repository_id}:issue:{number} (immutable repo ID, survives renames) and idempotency_key uses the content digest, so re-runs are safe.
• Defensive improvements: issue.get("labels", []) instead of issue["labels"], sort_keys=True in the digest serialization for deterministic ordering.
• Prompt security: the delegated prompt passes only the env-var name (token_name), never the value, instructs the agent never to print it, and scopes mutations to a single issue in a single repository.
• Tests are real, not mocks of mocks: the tests exercise the actual worker.py entrypoint via the github_automation_helpers fixture (which reads the manifest and loads the real bundle), mocking only the submit_subject_turn and urlopen boundaries.

Non-blocking finding

SKILL.md throughput description is stale. The last paragraph still reads:

Each scheduled run triages at most one changed issue per repository.

The old code used break after the first changed issue (one per run). The new code uses continue to skip already-triaged issues and submits all changed eligible issues per repository per run. The PR description itself says "For every changed eligible issue, it submits an idempotent subject turn," which matches the code. The SKILL.md text should be updated to reflect this (e.g., "Each scheduled run triages all changed eligible issues per repository"). This is documentation-only and does not block merge.

Verdict

No blocking findings. The architectural change is sound, the code is clean and well-tested, and the generated assets are consistent. Approving.

Generated by OpenHands AI on behalf of the user.

Re-review of rebased head (4c0c313)

I re-reviewed the current rebased head (factory/triage-subject-turns @ 4c0c313) against main, re-ran the full suite, and re-checked catalog/bundle consistency.

Verification performed

• Full test suite: 905 passed, 24 skipped (includes the 3 triage delivery tests).
• Ruff: clean on worker.py and test_github_triage_delivery.py.
• Generated catalog: npm run build:automations and npm run build:skills produce no diff — bundle-index.js and skills/index.js are in sync with source.
• Bundle-index ↔ source: worker.py and github_client.py in bundle-index.js match the source files byte-for-byte.

Re-review observations

The rebased head is content-identical to the head I approved previously — the rebase replayed cleanly onto the latest main with no conflicts or changes. The architectural refactor (scanner delegates to Automation's subject-turn API; no SDK imports, no workspace/conversation lifecycle in the worker) remains sound:

• submit_subject_turn() posts {source, subject_key, turn, idempotency_key} to AUTOMATION_SUBJECT_TURN_URL with a Bearer $AUTOMATION_RUN_TOKEN — no conversation/session/workspace machinery leaked into the scanner.
• Idempotency keys are {repository_id}:issue:{number} (immutable repo ID, survives renames) + content digest with sort_keys=True for deterministic ordering.
• The prompt passes only the env-var name (token_name), never the value, and scopes mutations to a single issue in a single repository.
• Tests exercise the real worker.py entrypoint via the github_automation_helpers fixture, mocking only the submit_subject_turn / urlopen boundaries.

Non-blocking finding (unchanged)

SKILL.md's last paragraph (and the inlined skills/index.js content) still says "Each scheduled run triages at most one changed issue per repository," but the code uses continue (not break) and submits all changed eligible issues per run. The PR description already states the correct behavior. Documentation-only; does not block merge.

Verdict

No blocking findings. Explicitly approving.

Generated by OpenHands AI on behalf of the user.

Re-review of rebased head (4c0c313)

I re-reviewed the current rebased head (factory/triage-subject-turns @ 4c0c313) against main, re-ran the full suite, and re-checked catalog/bundle consistency.

Verification performed

• Full test suite: 905 passed, 24 skipped (includes the 3 triage delivery tests).
• Ruff: clean on worker.py and test_github_triage_delivery.py.
• Generated catalog: npm run build:automations and npm run build:skills produce no diff — bundle-index.js and skills/index.js are in sync with source.
• Bundle-index ↔ source: worker.py and github_client.py in bundle-index.js match the source files byte-for-byte.

Re-review observations

The rebased head is content-identical to the head I approved previously — the rebase replayed cleanly onto the latest main with no conflicts or changes. The architectural refactor (scanner delegates to Automation's subject-turn API; no SDK imports, no workspace/conversation lifecycle in the worker) remains sound:

• submit_subject_turn() posts {source, subject_key, turn, idempotency_key} to AUTOMATION_SUBJECT_TURN_URL with a Bearer $AUTOMATION_RUN_TOKEN — no conversation/session/workspace machinery leaked into the scanner.
• Idempotency keys are {repository_id}:issue:{number} (immutable repo ID, survives renames) + content digest with sort_keys=True for deterministic ordering.
• The prompt passes only the env-var name (token_name), never the value, and scopes mutations to a single issue in a single repository.
• Tests exercise the real worker.py entrypoint via the github_automation_helpers fixture, mocking only the submit_subject_turn / urlopen boundaries.

Non-blocking finding (unchanged)

SKILL.md's last paragraph (and the inlined skills/index.js content) still says "Each scheduled run triages at most one changed issue per repository," but the code uses continue (not break) and submits all changed eligible issues per run. The PR description already states the correct behavior. Documentation-only; does not block merge.

Verdict

No blocking findings. Explicitly approving.

Generated by OpenHands AI on behalf of the user.

Re-review of head 05ad737 — per-issue failure-isolation fix

I reviewed the current head (factory/triage-subject-turns @ 05ad737) after the per-issue failure-isolation commit, re-ran the full suite, and re-checked catalog/bundle consistency.

What changed since the last approval

The per-issue submission logic was extracted from run() into a new _submit(self, repository_id, issue, issues) method, and the run() loop now wraps each call in a try/except:

for issue in sorted(issues, key=lambda item: item["number"]):
    try:
        self._submit(repository_id, issue, issues)
    except Exception as exc:
        print(
            f"Failed to submit {self.repository} issue "
            f"#{issue.get('number', '?')}: {exc}",
            flush=True,
        )

Previously a single submit_subject_turn failure (network error, 5xx from the Automation service, etc.) aborted the entire run, leaving all subsequent issues untriaged. Now each issue is isolated — a failure on one is logged and the remaining issues still get submitted. This mirrors the per-repository isolation already present in run_repositories (github_client.py).

The _submit method is a faithful extraction of the inline code: the early continue for already-triaged issues became an early return, which is equivalent. The idempotency keys, digest, marker, and prompt are unchanged.

Verification performed

• Full test suite: 906 passed, 24 skipped (includes the new test_triage_continues_after_one_submission_fails test, which verifies that a RuntimeError on issue #1 does not prevent issue #2 from being submitted).
• Ruff: 1 finding on worker.py (see non-blocking finding below). test_github_triage_delivery.py is clean.
• Generated catalog: npm run build:automations and npm run build:skills produce no diff — bundle-index.js and skills/index.js are in sync with source.
• Bundle-index ↔ source: worker.py and github_client.py in bundle-index.js match the source files byte-for-byte.
• SKILL.md: The previously-noted stale throughput description ("at most one changed issue per repository") is now corrected — it reads "Each scheduled run submits every changed eligible issue. A failure on one issue is reported and does not prevent the remaining issues from being submitted."

Non-blocking finding

Ruff BLE001 regression on worker.py. The new except Exception as exc: at line 120 lacks the # noqa: BLE001 suppression that the codebase's established pattern uses (github_client.py line 196: except Exception as exc: # noqa: BLE001 - one repository must not block others). Consequently ruff check worker.py now reports one error. Ruff is not enforced in CI, so this does not break the build, but it is a lint regression from the previously-clean state. The fix is one line — add # noqa: BLE001 - one issue must not block others to match the convention.

Verdict

No blocking findings. The failure-isolation fix is correct, minimal, well-tested, and the generated assets are consistent. Approving.

Generated by OpenHands AI on behalf of the user.